zvfs: improve libc FILE to integer fd abstraction #83386

cfriedt · 2024-12-25T21:49:00Z

Previously, there was an implicit assumption that Zephyr's internal struct fd_entry * was synonymous with FILE * from the C library.

This is generally not the case and aliasing these two distinct types was preventing a fair bit of functionality from Just Working - namely stdio function calls like fgets() and fopen() (which of course have historically lacked any kind of test coverage or official support in Zephyr).

The problem can be seen directly when trying to use a function like zvfs_fdopen().

Instead of aliasing the two types, require that all Zephyr C libraries provide

FILE *z_libc_file_alloc(int fd, const char *mode), to allocate and populate the required fields of a FILE object
int z_libc_file_get_fd(FILE *fp), to convert a FILE* object to an integer file descriptor.

For Picolibc and Newlib-based C libraries, these functions set and get the integer file descriptor from a field of the internal FILE object representation.

For the minimal C library, these functions convert between array index and struct fd_entry *.

Fixes #85013

cfriedt · 2024-12-25T23:33:03Z

@tagunil / @evgeniy-paltsev - ARC mwdt might need a similar change. I would be happy to include it here if you have suggestions.

tagunil · 2024-12-25T23:46:53Z

@tagunil / @evgeniy-paltsev - ARC mwdt might need a similar change. I would be happy to include it here if you have suggestions.

I generally like the idea, but I need time to check if/how it would work for our legacy MW libc.

keith-packard

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

cfriedt · 2024-12-26T06:02:14Z

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

Mostly by name. These functions can't depend on POSIX.

cfriedt · 2024-12-26T17:56:43Z

@tejlmand - do you know if there are similar changes necessary for the armclang toolchain? If so, I would be happy to include them here.

keith-packard · 2024-12-26T18:01:58Z

How does z_libc_file_alloc differ from the POSIX fdopen function? How does z_libc_file_get_fd differ from the POSIX fileno function?

Mostly by name. These functions can't depend on POSIX.

So, the picolibc (and newlib) implementations could just be wrappers?

cfriedt · 2024-12-26T22:25:42Z

Mostly by name. These functions can't depend on POSIX.

So, the picolibc (and newlib) implementations could just be wrappers?

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.

keith-packard · 2024-12-27T00:06:56Z

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.

#define _POSIX_C_SOURCE 200809L
#include <stdio.h>

FILE *z_libc_file_alloc(int fd, const char *mode)
{
    return fdopen(fd, mode);
}

int z_libc_file_get_fd(FILE *f)
{
    return fileno(f);
}

Alternatively, just add fdopen and fileno to the Zephyr API and use them directly.

cfriedt · 2024-12-27T00:25:50Z

What would the picolibc and newlib implementations be wrappers around? Maybe I've misunderstood the question.
#define _POSIX_C_SOURCE 200809L
#include <stdio.h>

FILE *z_libc_file_alloc(int fd, const char *mode)
{
    return fdopen(fd, mode);
}

int z_libc_file_get_fd(FILE *f)
{
    return fileno(f);
}
Alternatively, just add fdopen and fileno to the Zephyr API and use them directly.

That would introduce a POSIX dependency (as well as a dependency cycle), which is what we're trying to avoid in this situation.

It could maybe work if there were similar libc-internal functions that were not aliases of fdopen() or fileno().

In other words, here, we are limited to ISO C only and non-POSIX functions.

keith-packard · 2024-12-27T00:43:19Z

In other words, here, we are limited to ISO C only and non-POSIX functions.

So all we need to do is expose fileno and fdopen as Zephyr functions from the C library, just as we do for other Zephyr APIs which came from POSIX, like strnlen and strtok_r? For newlib, we could use the same hack used for those? Zero code added to Zephyr seems a lot better than copying code from an external library repository.

cfriedt · 2024-12-27T11:45:23Z

In other words, here, we are limited to ISO C only and non-POSIX functions.

So all we need to do is expose fileno and fdopen as Zephyr functions from the C library, just as we do for other Zephyr APIs which came from POSIX, like strnlen and strtok_r?

Not exactly - strnlen() and strtok_r() have zero side-effects and do not deal with management or validation of kernel resources.

Some of the kernel side of this might eventually hop the fence to syscall neighbourhood, and it seems like there would be some dependency cycles formed by calling userspace-facing APIs. From kernel space.

For newlib, we could use the same hack used for those? Zero code added to Zephyr seems a lot better than copying code from an external library repository.

I realize it's tempting to hack things, blur the lines and take shortcuts, for the sake of size-optimization, but I feel it's the wrong approach. Especially because "Zephyr is not a POSIX OS".

I think it's ok to call functions here that are not directly part of POSIX APIs though, so maybe if there are aliases that exist, those can be used instead (even though it's technically still the same code).

Here is a question: if, for example, only _write_r() and _write() are referenced directly, does the write() symbol get discarded if write() is an alias of _write()?

cfriedt · 2024-12-28T13:48:51Z

I tried using the aliases as well as the POSIX symbols here, but it unfortunately introduced an infinite recursion. I also tried using the option TC_PROVIDES_POSIX_DEVICE_IO with Picolibc and there were unresolved symbols.

Probably the existing change should suffice, for now.

cfriedt · 2024-12-28T14:08:12Z

fixed assignment of reentrant function pointers in the absence of consistent typing.

cfriedt · 2024-12-31T11:02:45Z

lib/os/fdtable.c

@@ -25,6 +25,11 @@
 #include <zephyr/internal/syscall_handler.h>
 #include <zephyr/sys/atomic.h>

+#ifndef CONFIG_MINIMAL_LIBC
+extern FILE *z_libc_file_alloc(int fd, const char *mode);


Is a better name here z_libc_fd_get_file() to highlight that it's the opposite of z_libc_file_get_fd()?

Naming things is hard, and it's really just an artifact of picolibc and newlib that they use calloc and malloc internally.

In theory, it should be easy enough to create a statically allocated table of FILE objects and have them use that instead of dynamic allocation.

I was looking through my local repo and found an implementation of fopen for Zephyr which doesn't use the underlying POSIX APIs at all. With your re-implementation of z_libc_file_get_file (nee fdopen), I wonder if we shouldn't provide both of these within Zephyr rather than within picolibc; we could then use a static array of FILE objects for both and you'd be able to easily translate between them.

The picolibc stdio implementation provides stable API which can support this, so it's not even a hack :-)

Check out https://github.com/keith-packard/zephyr/blob/picolibc-fopen/lib/libc/picolibc/fopen.c

I was looking through my local repo and found an implementation of fopen for Zephyr which doesn't use the underlying POSIX APIs at all.

That sounds promising.

With your re-implementation of z_libc_fd_get_file (nee fdopen), I wonder if we shouldn't provide both of these within Zephyr rather than within picolibc

That's sort of where I was heading as well. It's simple enough to implement inside of Zephyr and doesn't require any libc modifications.

It's also reasonably easy for other C libraries to do something similar, even those without POSIX support, like IAR.

The main difference between fdopen() and z_libc_fd_get_file() is that they have slightly different use-cases; the former is for calling from user code into the OS and is dependent on the POSIX API, whereas the latter is for calling into the libc from the OS, and doesn't depend on the POSIX API. So they're kind of going in opposite directions. The latter also eliminates possibly recursive calls.

we could then use a static array of FILE objects for both and you'd be able to easily translate between them.

That sounds great too - some Zephyr users prefer to avoid malloc-related things entirely. So that would be a bit more "user friendly" (for certain kinds of users).

The picolibc stdio implementation provides stable API which can support this, so it's not even a hack :-)

👍

@keith-packard - do you want to make a separate PR, or would you prefer if I added e.g. static FILE table here?

I'm happy to rename things as needed as well.

Some might prefer e.g. a __-prefix

The main difference between fdopen() and z_libc_fd_get_file() is that they have slightly different use-cases; the former is for calling from user code into the OS and is dependent on the POSIX API, whereas the latter is for calling into the libc from the OS, and doesn't depend on the POSIX API. So they're kind of going in opposite directions. The latter also eliminates possibly recursive calls.

There's nothing POSIX-specific about fdopen. On newlib, it doesn't use POSIX apis, it uses the existing
wrappers just like fopen does. With picolibc, it would be implemented entirely in Zephyr using non-POSIX Zephyr APIs. Using the POSIX name makes it far easier for developers to use, as long as the semantics are effectively the same. Creating a new name just makes this function harder to find.

That sounds great too - some Zephyr users prefer to avoid malloc-related things entirely. So that would be a bit more "user friendly" (for certain kinds of users).

Yup, I avoid using malloc in most of my embedded code too -- I find it far easier to reason about memory usage when it's all pre-allocated. However, in this case it sure looks like you're replacing one general allocator (malloc) with a special-purpose allocator (static array of FILE structs). I think the argument for doing that isn't very strong; you still need to evaluate the whole application to ensure there won't be any resource starvation.

I prefer to use APIs which don't rely on any dynamic allocation. Picolibc does this by allowing applications to declare FILE structs themselves, rather than calling a function to allocate them. The only problem with this approach is that stdio doesn't generally know about all of the in-use FILE structs, so fflush(NULL) doesn't work. We'd need some helpers to make that easy to use with Zephyr.

That's secondary to how fopen (and "fdopen") should be implemented in Zephyr; I think what we probably want is to implement "fdopen" (whatever the name ends up being) using the fdtable APIs and then implement fopen on top of that, just as picolibc does when using POSIX apis.

There's nothing POSIX-specific about fdopen.

Other than the fact that fdopen is entirely POSIX specific (it's not a part of ISO C).

With picolibc, it would be implemented entirely in Zephyr using non-POSIX Zephyr APIs.

That will be ideal, because then ISO C and POSIX would use the same underlying APIs.

Using the POSIX name makes it far easier for developers to use, as long as the semantics are effectively the same. Creating a new name just makes this function harder to find.

Uhm... I think you're a bit off there.

The point is that there is no dependency cycle. The OS shouldn't refer to a FILE <-> fd mapping managed by the C library if it uses the same name that application code uses. This is because that would be crossing the user-kernel API line twice, adding an API dependency cycle (which is a slippery slope). It's much better to have no cycles in the DAG of API calls.

By having a separate name for the kernel and for users, we mitigate the dependency cycle with a mutual dependency.

It's pretty simple. We will still offer both the POSIX API call and the native Zephyr call.

However, in this case it sure looks like you're replacing one general allocator (malloc) with a special-purpose allocator (static array of FILE structs).

A pool of objects is kind of like a special-purpose allocator, yes, but it's generally considered safer for certain applications than using heap allocation.

The main point is enabling standards-compliant code.

I think the argument for doing that isn't very strong; you still need to evaluate the whole application to ensure there won't be any resource starvation.

Technically, we could do declarative allocation as long as we can put FILE objects into the same linker section. Then it's effectively using both declarative allocation as well as pool-based allocation.

I'd like to eventually convert "files" to be proper k_objects soon. Likely that would require some linkage between the libc FILE and an new kernel object.

I prefer to use APIs which don't rely on any dynamic allocation.

That makes sense for most high-reliability / safety critical applications.

That's secondary to how fopen (and "fdopen") should be implemented in Zephyr

Yes.

cfriedt · 2025-01-01T19:07:59Z

The newlib/libc-hooks.c fixes were moved to #83481

tejlmand · 2025-01-08T10:21:57Z

@tejlmand - do you know if there are similar changes necessary for the armclang toolchain? If so, I would be happy to include them here.

will take a look. I think similar changes will be needed.

cfriedt · 2025-01-29T15:06:41Z

Have you had any time to look at this for armclang @tejlmand?

The function signature for open() in libc-hooks.c was incorrect. Additionally, the ALIAS_OF() macro causes a compile error, because it simplifies the open function signature to int open(), which is a conflicting definition. Use the correct definition to get around compile errors. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

Previously, there was an implicit assumption that Zephyr's internal struct fd_entry * was synonymous with FILE * from the C library. This is generally not the case and aliasing these two distinct types was preventing a fair bit of functionality from Just Working - namely stdio function calls like fgets() and fopen(). The problem count be seen directly when trying to use a function like zvfs_fdopen(). Instead of aliasing the two types, require that all Zephyr C libraries provide 1. FILE *z_libc_file_alloc(int fd, const char *mode) Allocate and populate the required fields of a FILE object 2. int z_libc_file_get_fd(FILE *fp) Convert a FILE* object to an integer file descriptor. For Picolibc and Newlib-based C libraries, these functions set and get the integer file descriptor from a field of the internal FILE object representation. For the minimal C library, these functions convert between array index and struct fd_entry pointers. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

lromeraj · 2025-03-10T17:41:26Z

Any additional updates on this? It should be ready to merge into the main branch, shouldn't it?

cfriedt · 2025-03-11T20:39:37Z

@lromeraj - it lacks reviews

jukkar · 2025-04-06T06:37:08Z

lib/libc/picolibc/z_libc.c

+	struct __file_bufio *bf;
+	int __maybe_unused unused;
+
+	bf = calloc(1, sizeof(struct __file_bufio) + BUFSIZ);


There was some comments about not using dynamic memory alloc. I just wonder if it could be possible to allow static allocation here, for example have Kconfig option to allow user either use dynamic alloc or to have an static array of these structs. This would allow the user to decide what suits his/hers usage better. Using malloc allows always a possibility to memory leaks.

github-actions · 2025-06-06T00:38:47Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch 2 times, most recently from de629fa to 6733b5d Compare December 25, 2024 22:05

cfriedt requested review from keith-packard, ycsin, stephanosio, tagunil and evgeniy-paltsev December 25, 2024 23:31

keith-packard reviewed Dec 26, 2024

View reviewed changes

cfriedt requested a review from tejlmand December 26, 2024 17:56

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch 2 times, most recently from 52c48ca to b1685eb Compare December 28, 2024 13:16

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from b1685eb to f1bdef2 Compare December 28, 2024 14:07

cfriedt commented Dec 31, 2024

View reviewed changes

cfriedt mentioned this pull request Jan 1, 2025

posix: implement FILE_LOCKING, SYSTEM_DATABASE, SYSTEM_DATABASE_R #83368

Draft

10 tasks

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch 3 times, most recently from f1bdef2 to a7f73fa Compare January 8, 2025 03:01

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from a7f73fa to 45d656d Compare January 8, 2025 03:04

This was referenced Jan 29, 2025

posix: options: implement the POSIX_FILE_LOCKING Option Group #84860

Draft

posix: options: implement POSIX_SYSTEM_DATABASE_R #84858

Draft

posix: options: implement POSIX_SYSTEM_DATABASE #84859

Draft

cfriedt marked this pull request as ready for review February 1, 2025 23:45

zephyrbot added area: C Library C Standard Library area: Base OS Base OS Library (lib/os) labels Feb 1, 2025

zephyrbot requested review from aescolar, andyross, dcpleung, nashif and peter-mitsis February 1, 2025 23:45

zephyrbot assigned stephanosio Feb 1, 2025

cfriedt added 2 commits February 16, 2025 14:40

cfriedt force-pushed the zvfs-improve-libc-file-and-int-fd-abstraction branch from 45d656d to 98991eb Compare February 16, 2025 19:43

cfriedt mentioned this pull request Feb 20, 2025

tests: posix: common: separate posix system database to standalone test #81476

Open

This was referenced Mar 4, 2025

tests: posix: common: separate posix threads base into a standalone test #81339

Open

tests: posix: add one integration platform per suite #86634

Closed

ycsin approved these changes Mar 18, 2025

View reviewed changes

jukkar reviewed Apr 6, 2025

View reviewed changes

github-actions bot added the Stale label Jun 6, 2025

cfriedt removed the Stale label Jun 6, 2025

zvfs: improve libc FILE to integer fd abstraction #83386

Are you sure you want to change the base?

zvfs: improve libc FILE to integer fd abstraction #83386

Conversation

cfriedt commented Dec 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfriedt commented Dec 25, 2024

Uh oh!

tagunil commented Dec 25, 2024

Uh oh!

keith-packard left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented Dec 26, 2024

Uh oh!

cfriedt commented Dec 26, 2024

Uh oh!

keith-packard commented Dec 26, 2024

Uh oh!

cfriedt commented Dec 26, 2024

Uh oh!

keith-packard commented Dec 27, 2024

Uh oh!

cfriedt commented Dec 27, 2024

Uh oh!

keith-packard commented Dec 27, 2024

Uh oh!

cfriedt commented Dec 27, 2024

Uh oh!

cfriedt commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfriedt commented Dec 28, 2024

Uh oh!

cfriedt Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-packard Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

cfriedt Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

keith-packard Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

cfriedt Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt commented Jan 1, 2025

Uh oh!

tejlmand commented Jan 8, 2025

Uh oh!

cfriedt commented Jan 29, 2025

Uh oh!

lromeraj commented Mar 10, 2025

Uh oh!

cfriedt commented Mar 11, 2025

Uh oh!

jukkar Apr 6, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 6, 2025

Uh oh!

Uh oh!

cfriedt commented Dec 25, 2024 •

edited

Loading

cfriedt commented Dec 28, 2024 •

edited

Loading

cfriedt Dec 31, 2024 •

edited

Loading

keith-packard Dec 31, 2024 •

edited

Loading

cfriedt Dec 31, 2024 •

edited

Loading